Migrate NVPs to their new keys #37556

iwiznia · 2024-02-29T18:51:30Z

~~Held on https://github.com/Expensify/Web-Expensify/pull/41082~~
~~Held on https://github.com/Expensify/Web-Expensify/pull/41300~~

Details

Fixed Issues

https://github.com/Expensify/Expensify/issues/372366

Tests

Check that priority mode, language, preferred skin tone and frequently used emojis are preserved and work properly
(no qa) Using the dev tools, check all data was migrated correctly to the new key and removed from the old key
Sign up as a new user, check the welcome banner is shown:

Offline tests

No

QA Steps

Same as tests

PR Author Checklist

I linked the correct issue in the ### Fixed Issues section above
I wrote clear testing steps that cover the changes made in this PR
- I added steps for local testing in the Tests section
- I added steps for the expected offline behavior in the Offline steps section
- I added steps for Staging and/or Production testing in the QA steps section
- I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
- I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
I included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  - If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
I verified that if a function's arguments changed that all usages have also been updated correctly
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

mkhutornyi · 2024-03-01T17:00:01Z

src/pages/NewChatPage.tsx

@@ -22,7 +22,7 @@ import * as Report from '@userActions/Report';
 import CONST from '@src/CONST';
 import ONYXKEYS from '@src/ONYXKEYS';
 import type * as OnyxTypes from '@src/types/onyx';
-import type {DismissedReferralBanners} from '@src/types/onyx/Account';
+import type DismissedReferralBanners from '@src/types/onyx/DismissedReferralBanners';


Suggested change

import type DismissedReferralBanners from '@src/types/onyx/DismissedReferralBanners';

Also, please apply these changes in this file:

- dismissedReferralBanners: DismissedReferralBanners; + dismissedReferralBanners: OnyxEntry<OnyxTypes.DismissedReferralBanners>; - shouldShowReferralCTA={!dismissedReferralBanners[CONST.REFERRAL_PROGRAM.CONTENT_TYPES.START_CHAT]} + shouldShowReferralCTA={!dismissedReferralBanners?.[CONST.REFERRAL_PROGRAM.CONTENT_TYPES.START_CHAT]}

melvin-bot · 2024-03-05T19:53:16Z

@grgia Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

iwiznia · 2024-03-05T22:11:20Z

This is on hold, but please review it

tgolen · 2024-03-06T03:54:43Z

src/ONYXKEYS.ts

@@ -111,22 +111,28 @@ const ONYXKEYS = {
    NVP_PRIORITY_MODE: 'nvp_priorityMode',

    /** Contains the users's block expiration (if they have one) */
-    NVP_BLOCKED_FROM_CONCIERGE: 'private_blockedFromConcierge',
+    NVP_BLOCKED_FROM_CONCIERGE: 'nvp_private_blockedFromConcierge',


It would really be great to group all these together under an NVP key like:

ONYXKEYS: { NVP: { BLOCKED_FROM_CONCIERGE: 'nvp_private_blockedFromConcierge', }, };

Or if not that, at least put all the NVPs next to each other in this file.

Moving them to be together mostly because I don't know how to make my IDE refactor this automatically and I don't want to have to update 420 files.

src/libs/migrations/NVPMigration.ts

+import Onyx from 'react-native-onyx';
+import ONYXKEYS from '@src/ONYXKEYS';
+
+const migrations = {


tgolen · 2024-03-06T04:02:48Z

src/libs/migrations/NVPMigration.ts

+                        Onyx.multiSet({
+                            [newKey]: value,
+                            [oldKey]: null,
+                        });


It's been a while since I've done the migrations, but do you know if it's a bad thing that this doesn't wait for the multiSet() promise to be resolved before calling resolveWhenDone()? I think it could technically mean that the migration finishes before the actual changes are committed to storage. If the app then tries to load in that state, it could cause issues (even fatal errors maybe).

Yes, I just noticed this in while writing the message above, fixing it.

tgolen · 2024-03-06T04:04:10Z

src/libs/migrations/NVPMigration.ts

+                key: oldKey,
+                callback: (value) => {
+                    Onyx.disconnect(connectionID);
+                    if (value !== null) {


NAB: I'm kind of surprised that there aren't lint warnings to switch these to early returns. I guess it's because there is no return statement, but I think it would be nice to follow the style of early returns so the code style is consistent.

I like this approach more because if I do early return I will need to call resolveWhenDone twice

Disregard, changing this given I need to pass resolveWhenDone to the promise too

src/libs/migrations/NVPMigration.ts

+            waitForCollectionCallback: true,
+            callback: (value) => {
+                Onyx.disconnect(connectionIDRecentlyUsedTags);
+                if (!value) {


Co-authored-by: Tim Golen <tgolen@gmail.com>

tgolen

Thanks for grouping the NVPs and switching to early returns!

grgia

code LGTM, my remaining comments are NAB

mkhutornyi

LGTM2! Just waiting for backend PR (fixing #37556 (comment)) to hit production

melvin-bot · 2024-03-18T13:25:27Z

@hayata-suenaga Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

melvin-bot · 2024-03-18T13:25:30Z

🎯 @mkhutornyi, thanks for reviewing and testing this PR! 🎉

An E/App issue has been created to issue payment here: #38470.

iwiznia · 2024-03-18T15:29:47Z

@mkhutornyi the reviewer checklist failed... maybe it is outdated or something?

mkhutornyi · 2024-03-18T15:32:10Z

It's success but don't merge yet. I still need to verify signup flow after backend deploy.

hayata-suenaga · 2024-03-18T18:47:57Z

seems like the PR has been already reviewed removing myself from the reviewer list 😄

iwiznia · 2024-03-19T21:55:50Z

@mkhutornyi can you retest now please?

@tgolen @grgia please re-review.

mkhutornyi

🟢

melvin-bot · 2024-03-20T15:43:40Z

@marcochavezf Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

OSBotify · 2024-03-20T16:09:25Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2024-03-22T16:19:51Z

🚀 Deployed to staging by https://github.com/iwiznia in version: 1.4.56-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2024-03-27T04:49:00Z

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.4.56-8 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Migrate NVPs to their new keys

50308c7

iwiznia self-assigned this Feb 29, 2024

iwiznia added 6 commits February 29, 2024 17:03

Remove nvp props from inside account

9ce6a3c

Fix usage of referral banners in account

55f816d

Suppress some errors

4c70029

Merge branch 'main' into ionatan_migratenvps

0b4e8db

Readd suppression

b17b23c

Merge branch 'main' into ionatan_migratenvps

e094189

iwiznia changed the title ~~Migrate NVPs to their new keys~~ [HOLD] Migrate NVPs to their new keys Mar 1, 2024

mkhutornyi suggested changes Mar 1, 2024

View reviewed changes

iwiznia added 5 commits March 1, 2024 14:14

Fix type errors

39d33de

More lints

3053b96

Merge branch 'main' into ionatan_migratenvps

6746721

Migrate recently used tags too

a2ada45

Make collection load properly

f0c5910

iwiznia marked this pull request as ready for review March 5, 2024 19:52

iwiznia requested a review from a team as a code owner March 5, 2024 19:52

iwiznia requested a review from tgolen March 5, 2024 19:53

melvin-bot bot removed the request for review from a team March 5, 2024 19:53

melvin-bot bot requested a review from grgia March 5, 2024 19:53

iwiznia added 2 commits March 5, 2024 16:57

Correct onyx key

fdadc74

Add nvp prefix

1290c36

tgolen requested changes Mar 6, 2024

View reviewed changes

Early return, move NVP constants, only resolve promise when set is done

54c7a4c

iwiznia requested a review from tgolen March 6, 2024 16:35

Update src/libs/migrations/NVPMigration.ts

be58c4f

Co-authored-by: Tim Golen <tgolen@gmail.com>

tgolen previously approved these changes Mar 6, 2024

View reviewed changes

iwiznia requested a review from mkhutornyi March 7, 2024 18:48

Tags

9962ab1

grgia previously approved these changes Mar 18, 2024

View reviewed changes

mkhutornyi approved these changes Mar 18, 2024

View reviewed changes

melvin-bot bot requested a review from hayata-suenaga March 18, 2024 13:25

melvin-bot bot mentioned this pull request Mar 18, 2024

Payment issue for E/App PR#37556 #38470

Closed

Change NVP name

df55f3c

iwiznia dismissed grgia’s stale review via df55f3c March 18, 2024 15:32

iwiznia requested review from tgolen and grgia March 18, 2024 15:32

hayata-suenaga removed their request for review March 18, 2024 18:47

iwiznia changed the title ~~Migrate NVPs to their new keys~~ [HOLD] Migrate NVPs to their new keys Mar 18, 2024

iwiznia changed the title ~~[HOLD] Migrate NVPs to their new keys~~ Migrate NVPs to their new keys Mar 19, 2024

Merge from main

7b08292

Lint

df49722

grgia approved these changes Mar 20, 2024

View reviewed changes

mkhutornyi approved these changes Mar 20, 2024

View reviewed changes

melvin-bot bot requested a review from marcochavezf March 20, 2024 15:43

iwiznia merged commit bc9eb16 into main Mar 20, 2024
16 checks passed

iwiznia deleted the ionatan_migratenvps branch March 20, 2024 16:03

github-actions bot mentioned this pull request Mar 22, 2024

Deploy Checklist: New Expensify 2024-03-22 #38811

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate NVPs to their new keys #37556

Migrate NVPs to their new keys #37556

iwiznia commented Feb 29, 2024 •

edited

Loading

mkhutornyi Mar 1, 2024

mkhutornyi Mar 1, 2024

melvin-bot bot commented Mar 5, 2024

iwiznia commented Mar 5, 2024

tgolen Mar 6, 2024

iwiznia Mar 6, 2024

This comment was marked as resolved.

tgolen Mar 6, 2024

iwiznia Mar 6, 2024

tgolen Mar 6, 2024

iwiznia Mar 6, 2024

iwiznia Mar 6, 2024

This comment was marked as resolved.

tgolen left a comment

grgia left a comment

mkhutornyi left a comment

melvin-bot bot commented Mar 18, 2024

melvin-bot bot commented Mar 18, 2024

iwiznia commented Mar 18, 2024

mkhutornyi commented Mar 18, 2024

hayata-suenaga commented Mar 18, 2024

iwiznia commented Mar 19, 2024

mkhutornyi left a comment

melvin-bot bot commented Mar 20, 2024

OSBotify commented Mar 20, 2024

OSBotify commented Mar 22, 2024

OSBotify commented Mar 27, 2024

Migrate NVPs to their new keys #37556

Migrate NVPs to their new keys #37556

Conversation

iwiznia commented Feb 29, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melvin-bot bot commented Mar 5, 2024

iwiznia commented Mar 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

tgolen left a comment

Choose a reason for hiding this comment

grgia left a comment

Choose a reason for hiding this comment

mkhutornyi left a comment

Choose a reason for hiding this comment

melvin-bot bot commented Mar 18, 2024

melvin-bot bot commented Mar 18, 2024

iwiznia commented Mar 18, 2024

mkhutornyi commented Mar 18, 2024

hayata-suenaga commented Mar 18, 2024

iwiznia commented Mar 19, 2024

mkhutornyi left a comment

Choose a reason for hiding this comment

melvin-bot bot commented Mar 20, 2024

OSBotify commented Mar 20, 2024

OSBotify commented Mar 22, 2024

OSBotify commented Mar 27, 2024

iwiznia commented Feb 29, 2024 •

edited

Loading